Skip to content

Issue #46589 - Kill borrows on a local variable whenever we assign ov… #46752

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Dec 22, 2017
Merged

Issue #46589 - Kill borrows on a local variable whenever we assign ov… #46752

merged 1 commit into from
Dec 22, 2017

Conversation

Yoric
Copy link
Contributor

@Yoric Yoric commented Dec 15, 2017

…er this variable

This is a first patch for the issue, handling the simple case while I figure out the data structures involved in the more complex cases.

@rust-highfive
Copy link
Contributor

r? @pnkfelix

(rust_highfive has picked a reviewer for you, use r? to override)

@bjorn3
Copy link
Member

bjorn3 commented Dec 16, 2017

Issue #46589 (just to link it)

Could you add a test?

@pnkfelix
Copy link
Contributor

The code looks fine to me; just add a test and we should be good.

@pnkfelix pnkfelix added A-NLL Area: Non-lexical lifetimes (NLL) WG-compiler-nll S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 16, 2017
@Yoric
Copy link
Contributor Author

Yoric commented Dec 19, 2017 •
edited
Loading

I have added the test of issue #46589 and much to my surprise, it still fails, although it looks like it should be covered by my patch. I must be missing something obvious:

error[E0506]: cannot assign to `list` because it is borrowed
  --> /Users/david/Documents/Code/rust-nll/src/test/run-pass/nll-iterating-and-updating.rs:25:13
   |
23 |         result.push(&mut list.value);
   |                     --------------- borrow of `list` occurs here
24 |         if let Some(n) = list.next.as_mut() {
25 |             list = n;
   |             ^^^^^^^^ assignment to borrowed `list` occurs here

error: aborting due to previous error

@Yoric
Copy link
Contributor Author

Yoric commented Dec 19, 2017 •
edited
Loading

Note: problem appears even if we add a

let mut list = list;

so it's probably not caused by list being an argument.

@arielb1
Copy link
Contributor

arielb1 commented Dec 19, 2017 •
edited
Loading

@Yoric

I think that's because you haven't fixed this:

rust/src/librustc_mir/borrow_check/mod.rs

Lines 335 to 355 in a9f047c

StatementKind::Assign(ref lhs, ref rhs) => {
// NOTE: NLL RFC calls for *shallow* write; using Deep
// for short-term compat w/ AST-borrowck. Also, switch
// to shallow requires to dataflow: "if this is an
// assignment `place = <rvalue>`, then any loan for some
// path P of which `place` is a prefix is killed."
self.mutate_place(
ContextKind::AssignLhs.new(location),
(lhs, span),
Deep,
JustWrite,
flow_state,
);
self.consume_rvalue(
ContextKind::AssignRhs.new(location),
(rhs, span),
location,
flow_state,
);
}

@Yoric
Copy link
Contributor Author

Yoric commented Dec 19, 2017

Ok, thanks to @arielb1, this now seems to work.

@Yoric Yoric force-pushed the nll branch 2 times, most recently from a96ad49 to 5e0e633 Compare December 19, 2017 18:36
@Yoric
Copy link
Contributor Author

Yoric commented Dec 19, 2017

(oops, forgot to remove the obsolete test, this should now be fixed)

@arielb1
Copy link
Contributor

arielb1 commented Dec 19, 2017

@bors r+

@bors
Copy link
Collaborator

bors commented Dec 19, 2017

📌 Commit 5e0e633 has been approved by arielb1

@kennytm kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 20, 2017
@bors
Copy link
Collaborator

bors commented Dec 20, 2017

⌛ Testing commit 5e0e633d1f780f865b4c8298f7116b274824994a with merge 627022b2338aeb9e7d416bb1c6fee34125d8ec32...

@bors
Copy link
Collaborator

bors commented Dec 20, 2017

💔 Test failed - status-appveyor

@Yoric
Copy link
Contributor Author

Yoric commented Dec 21, 2017

As far as I can tell, the single failure is a timeout, which I imagine is not caused by this PR.

@kennytm
Copy link
Member

kennytm commented Dec 21, 2017 •
edited
Loading

@bors retry — 3 hour timeout.

Build number 1.0.5591 1.0.5765
make-prepare 43.49 49.60
pytest/bootstrap 0.00 0.00
stage0-std 112.40 120.96
stage0-tidy 6.55 7.46
bootstrap 5.73 5.92
stage0-test 11.26 12.68
llvm 557.90 642.91
stage0-rustc 1080.53 1471.46
stage1-std 54.30 75.27
stage1-test 16.18 21.58
stage1-rustc 1357.72 2055.58
stage0-compiletest 54.95 103.39
test/ui 73.17 188.49
test/run-pass 1380.91 1874.05
test/compile-fail 573.45 568.06
test/parse-fail 26.47 32.96
test/run-fail 30.84 37.61
test/run-pass-valgrind 4.08 4.66
test/mir-opt 19.00 33.14
test/codegen 9.00 10.73
test/codegen-units 12.81 10.29
test/incremental 81.99 78.64
test/debuginfo 31.47 31.93
test/ui-fulldeps 17.09 32.71
test/run-pass-fulldeps 223.00 297.01
test/run-fail-fulldeps 3.08 4.81
test/compile-fail-fulldeps 69.19 91.50
stage2-rustdoc 228.74 301.32
test/run-make 144.75 171.07
test/rustdoc 121.66 175.18
stage1-test-libstd 178.30 236.88
test/libstd 790.40 914.47
stage1-test-libtest 12.46 13.91
test/libtest 13.82 18.38
stage1-test-librustc 309.59 394.63
test/librustc 314.23 400.63
stage2-test-rustdoc 45.49 54.72
test/libtools 46.40 55.56
stage0-unstable-book-gen 8.60 10.47
stage0-rustbook 177.15 181.70
doc/std 27.87 36.66
doc/compiler 1.42 3.33
stage2-error_index_generator 22.21 28.20
stage0-linkchecker 2.51 2.92
test/linkchecker 44.02 48.44
test/docs 241.15  
stage2-test-error-index 61.73  

 

@arielb1 arielb1 mentioned this pull request Dec 21, 2017
Closed
@Yoric
Copy link
Contributor Author

Yoric commented Dec 21, 2017

Fwiw, SYS_BITS=32, RUST_CONFIGURE_ARGS=--build=i686-pc-windows-gnu, SCRIPT=python x.py test, MINGW_URL=https://s3-us-west-1.amazonaws.com/rust-lang-ci2/rust-ci-mirror, MINGW_ARCHIVE=i686-6.3.0-release-posix-dwarf-rt_v5-rev2.7z, MINGW_DIR=mingw32 timed out after 3 hr

@bors
Copy link
Collaborator

bors commented Dec 21, 2017

☔ The latest upstream changes (presumably #46904) made this pull request unmergeable. Please resolve the merge conflicts.

@kennytm kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 21, 2017
@kennytm
Copy link
Member

kennytm commented Dec 21, 2017

@bors r=arielb1

@bors
Copy link
Collaborator

bors commented Dec 21, 2017

📌 Commit fcb1090 has been approved by arielb1

@kennytm kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 21, 2017
@bors
Copy link
Collaborator

bors commented Dec 22, 2017

⌛ Testing commit fcb1090 with merge 264af16...

bors added a commit that referenced this pull request Dec 22, 2017
@bors
Auto merge of #46752 - Yoric:nll, r=arielb1
264af16 
Issue #46589 - Kill borrows on a local variable whenever we assign ov…

…er this variable

This is a first patch for the issue, handling the simple case while I figure out the data structures involved in the more complex cases.
@bors
Copy link
Collaborator

bors commented Dec 22, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: arielb1
Pushing 264af16 to master...

@bors bors merged commit fcb1090 into rust-lang:master Dec 22, 2017
@matthewjasper matthewjasper mentioned this pull request Dec 23, 2017
Merged
bors added a commit that referenced this pull request Dec 26, 2017
@bors
Auto merge of #46975 - matthewjasper:mir-moveck-asm, r=arielb1
cddc4a6 
[MIR Borrowck] Moveck inline asm statements

Closes #45695

New behavior:
* Input operands to `asm!` are moved, direct output operands are initialized.
* Direct, non-read-write outputs match the assignment changes in #46752 (Shallow writes, end borrows).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@pnkfelix pnkfelix

Labels
A-NLL Area: Non-lexical lifetimes (NLL) S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Yoric @rust-highfive @bjorn3 @pnkfelix @arielb1 @bors @kennytm